dns: Refactor forEach to map #5803

benjamingr · 2016-03-19T18:24:26Z

Pull Request check-list

Affected core subsystem(s)

dns

Description of change

This is (as suggested) a more modular take on - #5762 , adding the changes one by one and making sure they're less objectionable. Making more smaller PRs.

Refactor a forEach to a map in the setServers function of the
dns module - simplifying the code. In addition, use more descriptive
variable names and const over var where possible.

bnoordhuis · 2016-03-19T18:29:05Z

Maybe I missed the memo but why is Array#map better than Array#forEach?

Aside, mixing several style changes in a single commit is kind of meh.

benjamingr · 2016-03-19T18:32:15Z

@bnoordhuis I tried to do as little unrelated changes as possible here and only edited one function.

Basically, the code was creating a new array, and then pushing to it in a loop and returning the results which is what Array#map already does. We were re-implementing map in the code.

I think map is easier to reason about in this case and it's more obvious what it does and why.

ChALkeR · 2016-03-19T19:30:09Z

map is indeed easier to read here to me.

benjamingr · 2016-03-19T19:55:02Z

@ChALkeR care to review?

ChALkeR · 2016-03-19T20:08:23Z

lib/dns.js


    throw new Error('IP address is not properly formatted: ' + serv);
  });

-  var r = cares.setServers(newSet);
+  const errorNumber = cares.setServers(newSet);


Does this always return a number?

Yes, it does. This is why I think r was confusing. It's SetServers in cares_wrap.cc which does:

args.GetReturnValue().Set(err);

where err is either 0 for no error or a non-zero value for an error, where err is an int.

cjihrig · 2016-03-20T02:48:08Z

-1 to all the variable renaming. It makes this harder to review alongside the other changes.

benjamingr · 2016-03-20T08:06:48Z

Fair enough, I'll revert all the naming changes except for ipVersion and errorNumber is that acceptable @cjihrig or would you prefer I reverted those as well? I just think r and ver are really confusing variable names and I personally had a hard time making sure I understood what they meant when I've read them.

YurySolovyov · 2016-03-20T08:08:22Z

I think .map is also pre-allocating array where looping + pushing might require array resize internally.

ChALkeR · 2016-03-20T11:48:12Z

lib/dns.js

-
-    var match = serv.match(/\[(.*)\](:\d+)?/);
+    if (ipVersion !== 0)
+      return [ver, serv];


Where did ver come from? ;-)

benjamingr · 2016-03-21T07:52:30Z

@ChALkeR I reverted most of the variable names as it seems people didn't like that change being included in this one. Sorry for the mess :)

ChALkeR · 2016-03-21T11:12:27Z

lib/dns.js


-    if (ver)
-      return newSet.push([ver, s]);
+    if (ipVersion)


This is inconsistent with if (ipVersion !== 0) in two places above.

benjamingr · 2016-03-21T11:45:56Z

@ChALkeR fixed. Thanks.

cjihrig · 2016-03-21T13:09:47Z

LGTM if the CI is happy.

jasnell · 2016-03-21T16:08:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1995/

jasnell · 2016-03-21T16:08:51Z

@benjamingr ... looks like this needs a rebase and update.

benjamingr · 2016-03-21T18:59:04Z

Yeah, on top of my other (now landed) PR, I'll do it today and tomorrow and re-run CI, thanks :)

ChALkeR · 2016-03-22T09:48:19Z

lib/dns.js

    // reset the servers to the old servers, because ares probably unset them
    cares.setServers(orig.join(','));

    var err = cares.strerror(r);
    throw new Error(`c-ares failed to set servers: "${err}" [${servers}]`);
+


Unrelated newline? Is that accidental?

Yeah, that's because during the rebase I'll fix it.

ChALkeR · 2016-03-22T09:48:28Z

LGTM

jasnell · 2016-03-22T14:44:49Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/2020/

jasnell · 2016-03-22T15:24:47Z

Looks like there's a linting issue on this: https://ci.nodejs.org/job/node-test-linter/1770/console

benjamingr · 2016-03-22T15:27:46Z

Yeah, pushed a fix, any idea about the other issue? Is that just the build trolling?

jasnell · 2016-03-22T15:28:50Z

Not sure, looks like an unrelated timeout. It's the first time I've seen it tho. /cc @Trott

targos · 2016-03-22T15:29:33Z

You also pushed a build/config.gypi file with your changes

Refactor a forEach to a `map` in the `setServers` function of the dns module - simplifying the code. In addition, use more descriptive variable names and `const` over `var` where possible. PR-URL: nodejs#5803 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

benjamingr · 2016-03-22T15:34:33Z

Thanks, running a new CI https://ci.nodejs.org/job/node-test-pull-request/2024/

jasnell · 2016-03-22T16:33:23Z

One failure in CI looks unrelated but can you please verify.

benjamingr · 2016-03-22T17:15:48Z

https://ci.nodejs.org/job/node-test-binary-arm/1451/RUN_SUBSET=5,nodes=pi1-raspbian-wheezy/console @jasnell looks unrelated, pinging @nodejs/build so they see the link.

Refactor a forEach to a `map` in the `setServers` function of the dns module - simplifying the code. In addition, use more descriptive variable names and `const` over `var` where possible. PR-URL: nodejs#5803 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

benjamingr · 2016-03-22T19:37:46Z

Landed in 4985c34 thanks!

Refactor a forEach to a `map` in the `setServers` function of the dns module - simplifying the code. In addition, use more descriptive variable names and `const` over `var` where possible. PR-URL: #5803 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

MylesBorins · 2016-04-08T21:19:41Z

@benjamingr this patch does not land cleanly. Would you be able to backport it?

benjamingr · 2016-04-10T11:39:52Z

@thealphanerd I'm not sure we should backport it and in either case it's just a code cleanup. I can backport it though. I'll try to do a sweep through my PRs to master and do all the backporting to 5.x and 4.x next weekend.

MylesBorins · 2016-04-11T15:04:48Z

thanks @benjamingr

benjamingr added the dns Issues and PRs related to the dns subsystem. label Mar 19, 2016

ChALkeR reviewed Mar 19, 2016
View reviewed changes

ChALkeR reviewed Mar 20, 2016
View reviewed changes

lib/dns.js

var match = serv.match(/\[(.*)\](:\d+)?/);

if (ipVersion !== 0)

return [ver, serv];

Copy link

Member

ChALkeR Mar 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did ver come from? ;-)

benjamingr force-pushed the dns-foreach-to-map branch from 384dfdc to 3747a13 Compare March 20, 2016 12:05

ChALkeR reviewed Mar 21, 2016
View reviewed changes

benjamingr force-pushed the dns-foreach-to-map branch from aa63331 to 1a96099 Compare March 22, 2016 09:41

benjamingr force-pushed the dns-foreach-to-map branch from 1a96099 to 568b586 Compare March 22, 2016 09:42

ChALkeR reviewed Mar 22, 2016
View reviewed changes

benjamingr force-pushed the dns-foreach-to-map branch from 568b586 to 797a272 Compare March 22, 2016 09:57

benjamingr mentioned this pull request Mar 22, 2016

dns: refactor dns to more modern JavaScript #5762

Closed

benjamingr force-pushed the dns-foreach-to-map branch from b40d64c to 669db03 Compare March 22, 2016 15:33

Trott mentioned this pull request Mar 22, 2016

Investigate flaky test-dgram-send-callback-buffer-length #5848

Closed

benjamingr closed this Mar 22, 2016

jasnell added the lts-watch-v4.x label Mar 22, 2016

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels May 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: Refactor forEach to map #5803

dns: Refactor forEach to map #5803

benjamingr commented Mar 19, 2016

bnoordhuis commented Mar 19, 2016

benjamingr commented Mar 19, 2016

ChALkeR commented Mar 19, 2016

benjamingr commented Mar 19, 2016

ChALkeR Mar 19, 2016

benjamingr Mar 20, 2016

cjihrig commented Mar 20, 2016

benjamingr commented Mar 20, 2016

YurySolovyov commented Mar 20, 2016

ChALkeR Mar 20, 2016

benjamingr commented Mar 21, 2016

ChALkeR Mar 21, 2016

benjamingr commented Mar 21, 2016

cjihrig commented Mar 21, 2016

jasnell commented Mar 21, 2016

jasnell commented Mar 21, 2016

benjamingr commented Mar 21, 2016

ChALkeR Mar 22, 2016

benjamingr Mar 22, 2016

ChALkeR commented Mar 22, 2016

jasnell commented Mar 22, 2016

jasnell commented Mar 22, 2016

benjamingr commented Mar 22, 2016

jasnell commented Mar 22, 2016

targos commented Mar 22, 2016

benjamingr commented Mar 22, 2016

jasnell commented Mar 22, 2016

benjamingr commented Mar 22, 2016

benjamingr commented Mar 22, 2016

MylesBorins commented Apr 8, 2016

benjamingr commented Apr 10, 2016

MylesBorins commented Apr 11, 2016

dns: Refactor forEach to map #5803

dns: Refactor forEach to map #5803

Conversation

benjamingr commented Mar 19, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

bnoordhuis commented Mar 19, 2016

benjamingr commented Mar 19, 2016

ChALkeR commented Mar 19, 2016

benjamingr commented Mar 19, 2016

ChALkeR Mar 19, 2016

Choose a reason for hiding this comment

benjamingr Mar 20, 2016

Choose a reason for hiding this comment

cjihrig commented Mar 20, 2016

benjamingr commented Mar 20, 2016

YurySolovyov commented Mar 20, 2016

ChALkeR Mar 20, 2016

Choose a reason for hiding this comment

benjamingr commented Mar 21, 2016

ChALkeR Mar 21, 2016

Choose a reason for hiding this comment

benjamingr commented Mar 21, 2016

cjihrig commented Mar 21, 2016

jasnell commented Mar 21, 2016

jasnell commented Mar 21, 2016

benjamingr commented Mar 21, 2016

ChALkeR Mar 22, 2016

Choose a reason for hiding this comment

benjamingr Mar 22, 2016

Choose a reason for hiding this comment

ChALkeR commented Mar 22, 2016

jasnell commented Mar 22, 2016

jasnell commented Mar 22, 2016

benjamingr commented Mar 22, 2016

jasnell commented Mar 22, 2016

targos commented Mar 22, 2016

benjamingr commented Mar 22, 2016

jasnell commented Mar 22, 2016

benjamingr commented Mar 22, 2016

benjamingr commented Mar 22, 2016

MylesBorins commented Apr 8, 2016

benjamingr commented Apr 10, 2016

MylesBorins commented Apr 11, 2016